Skip to content

Port to jdk17 and spring 3.0.0 #1279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2021
Merged

Port to jdk17 and spring 3.0.0 #1279

merged 1 commit into from
Dec 9, 2021

Conversation

mikereiche
Copy link
Collaborator

Closes #1278.

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@mikereiche mikereiche requested review from daschl and mp911de December 8, 2021 04:15
@wilkinsona wilkinsona mentioned this pull request Dec 8, 2021
Closed
mp911de
mp911de requested changes Dec 8, 2021
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments. Also, you might want to target the 5.0.x branch with this PR.

src/main/java/org/springframework/data/couchbase/repository/query/N1qlQueryCreator.java Outdated

Class<?> returnedObjectType = queryMethod.getReturnedObjectType();
if (ClassUtils.isPrimitiveOrWrapper(returnedObjectType)) {
this.entity = new BasicCouchbasePersistentEntity(ClassTypeInformation.from(returnedObjectType));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entities should not be generally created for return types. A query is always contextually related to an entity, but using the return type doesn't match that assumption. You could return void or Integer and entity creation inspects constructors etc. That could should be adopted properly to detect whether the return type qualifies as entity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was specifically added to handle methods of the type Long countByxxxx()- the existing code was failing following the the jdk16 enforcement of InaccessibleObjectException. The code here corresponds to https://github.com/spring-projects/spring-data-mongodb/blob/7a640256698f0772c1509c58e4bc71652a448567/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoQueryMethod.java#L143

I'll see if there is a better way to handle this - at least factor out the common code that is repeated in multiple places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yuck! I see, the code in MongoDB isn't even much better although it uses the EntityMetadata approach. Creating PersistentEntity objects can easily lead to InaccessibleObjectException and my comment above is motivated by using code that isn't likely to lead to InaccessibleObjectException.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cases where I had made changes were hitting exceptions because of a latent bug - they were using the returnType of the method where they should have been using the entityType. After fixing that bug, the subsequent getPersistentEntity( type ) always returns an existing entity. I think we are ok here.

...ain/java/org/springframework/data/couchbase/repository/query/StringBasedN1qlQueryParser.java Outdated
PersistentEntity persistentEntity = couchbaseConverter.getMappingContext().getPersistentEntity(resultClass);
PersistentEntity persistentEntity;
if (ClassUtils.isPrimitiveOrWrapper(resultClass)) {
persistentEntity = new BasicCouchbasePersistentEntity(ClassTypeInformation.from(resultClass));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar issue as with N1qlQueryCreator. The entity should be determined on the query method level and if the return type qualifies as entity, the entity to be used should be refined into the type instead of creating PersistentEntity instances.

...ain/java/org/springframework/data/couchbase/repository/query/StringBasedN1qlQueryParser.java Outdated
@@ -207,7 +215,12 @@ private void getProjectedFieldsInternal(String bucketName, CouchbasePersistentPr

if (resultClass != null) {
Set<String> fieldList = fields != null ? new HashSet<>(Arrays.asList(fields)) : null;
PersistentEntity persistentEntity = couchbaseConverter.getMappingContext().getPersistentEntity(resultClass);
PersistentEntity persistentEntity;
if (ClassUtils.isPrimitiveOrWrapper(resultClass.getType())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar issue as with N1qlQueryCreator

@mp911de
Copy link
Member

mp911de commented Dec 8, 2021

Paging @wilkinsona for keeping track of the Couchbase 5.0 progress.

@mikereiche mikereiche changed the base branch from main to 5.0.x December 8, 2021 18:50
@mikereiche mikereiche force-pushed the datacouch_1278_jdk17 branch from 15ff22f to 0198ffd Compare December 9, 2021 01:11
mp911de
mp911de approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to rename src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension to jakarta.enterprise.inject.spi.Extension.

Feel free to merge this PR afterward. Thanks for your support!

pom.xml
<java-module-name>spring.data.couchbase</java-module-name>
<jodatime>2.10.13</jodatime>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've dropped support for Jodatime in Spring Data Commons. Unless there's a strong reason to keep Joda, we suggest switching to JSR-310 types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this as optional - if the user provides the jodatime then we will support it. I believe the JSR-310 types are handled by https://github.com/spring-projects/spring-data-couchbase/blob/5.0.x/src/main/java/org/springframework/data/couchbase/core/convert/CouchbaseJsr310Converters.java

@mikereiche
Port to jdk17 and spring 3.0.0
9364661 
This also allows support of repository methods that return a simple type.
i.e.
@ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS)
@query("SELECT iata, \"\" as __id, 0 as __cas from #{#n1ql.bucket} WHERE #{#n1ql.filter}")
List<String> getStrings();

Closes #1278.
@mikereiche mikereiche force-pushed the datacouch_1278_jdk17 branch from 0198ffd to 9364661 Compare December 9, 2021 19:06
@mikereiche mikereiche merged commit 49e7de0 into 5.0.x Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de approved these changes

@daschl daschl Awaiting requested review from daschl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade Java baseline to Java 17
2 participants
@mikereiche @mp911de